Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

removeAttrs: Added optional value filter #977

Closed
wants to merge 1 commit into from
Closed

removeAttrs: Added optional value filter #977

wants to merge 1 commit into from

Conversation

Herman-Freund
Copy link

@Herman-Freund Herman-Freund commented Jun 6, 2018 •
edited
Loading

Example:

Remove all "fill" attributes that the value is "none"

{
  removeAttrs: {
    attrs: '*:fill:none'
  }
}

Remove all "fill" attributes where the value is NOT "none"

{
  removeAttrs: {
    attrs: '*:fill:((?!^none$).)*'
  }
}

Remove all "fill" attributes except when the value is white (white, #fff, rgb(255,255,255), etc.)

attrs: [
  "*:fill:((?!^\\s*white\\s*$|^\\s*#(f{3}|f{4}|f{6}|f{8})\\s*$|^\\s*rgba?\\(\\s*(255|100%)\\s*([,\\s]\\s*\\3\\s*){2}([,\\s]\\s*(1|100%)\\s*)?\\)\\s*$|^\\s*hsla?\\(\\s*[\\w.]+\\s*[,\\s]\\s*\\d{1,3}%\\s*[,\\s]\\s*100%\\s*([/,\\s]\\s*(1|100%)\\s*)?).)*",
  ...
]

@Herman-Freund Herman-Freund changed the title Added optional value filter removeAttr: Added optional value filter Jun 6, 2018
@Herman-Freund Herman-Freund mentioned this pull request Jun 6, 2018
Closed
@Herman-Freund Herman-Freund changed the title removeAttr: Added optional value filter removeAttrs: Added optional value filter Jun 13, 2018
@Herman-Freund Herman-Freund force-pushed the master branch 2 times, most recently from 8f40cd0 to 90f8ba7 Compare June 15, 2018 15:29
@Herman-Freund
Copy link
Author

@GreLI How can we move this forward? Will pinging the original creator of this plugin help (@bennyschudel)?

Thanks

@frebro
Copy link

frebro commented Jun 21, 2018 •
edited
Loading

This would be a very useful addition to the plugin.

This was referenced Jul 11, 2018
Merged
Closed
roblevintennis
roblevintennis reviewed Jul 12, 2018
View reviewed changes
plugins/removeAttrs.js Outdated
@@ -81,12 +90,16 @@ exports.fn = function(item, params) {
// prepare patterns
var patterns = params.attrs.map(function(pattern) {

// apply to all elements if specifc element is omitted
// if no element separators (:), assume it's attribute name, and apply to all elements and regardless of value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe should say "to all element regardless of value" (I just removed the extraneous 'and')

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe but I was trying to emphasize that the plugin will make 2 assumptions if there are no separators, 1) match all elements and 2) match any value.

Copy link
Contributor

@roblevintennis roblevintennis Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

assume it's an attribute name, and apply the transformations to all elements (regardless of their value)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't parentheses imply "lesser importance" (e.g. afterthought)? Which is not exactly the message intended to convey.

Thanks

Copy link
Contributor

@roblevintennis roblevintennis Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you could state something like:

if no element separators (:), apply the pattern match against attribute name, otherwise matching on all elements and values

or

if no element separators (:), match on all elements and values applying the pattern match against attribute name specifically

Both would seem to express the functionality of the regex your join will result in.

Copy link
Author

@Herman-Freund Herman-Freund Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@roblevintennis
Copy link
Contributor

I too think this would be a nice addition to the plugin...Herman chimed in on my PR #999 which is a parity feature with grunt-svgstore and allows for using the SVG trick to style sprites with one more color by using the currentColor value to inherit font's color. That said, I could alternatively use his changes here to achieve with regex filtering {"attrs":"*:(stroke|fill):((?!^currentColor$).)*"} so that's a definite use case for getting this in.

Here's an article I wrote on CSS tricks which explains the use case aforementioned:
https://css-tricks.com/gotchas-on-getting-svg-into-production/#article-header-id-11

@raduflp
Copy link

raduflp commented Sep 24, 2018

any plans to merge this PR in the near future?

@MattMakes
Copy link

@deepsweet @GreLI Any way to get this moved through?

@Jetski5822
Copy link

Any way this can be merged in?

@roblevintennis
Copy link
Contributor

It's hard to not wonder if SVGO is really maintained at this point with no feedback from core folk for nearly 6 months. It's such a good tool I certainly hope that is not the case. @Herman-Freund did you ever hear any feedback outside of this channel on your work? I can't see why we wouldn't want this feature added esp. given the positive comments from various people here.

@Herman-Freund
Copy link
Author

@Herman-Freund did you ever hear any feedback outside of this channel on your work?

No 😕

Thanks everyone for the positive feedback!

@Herman-Freund
Copy link
Author

@GreLI
Other than #1055, are there any reasons for not merging this?

Thanks

GreLI added a commit to GreLI/svgo that referenced this pull request Feb 24, 2019
@GreLI
removeAttrs: add optional value filter (svg#977)
d23355e
@GreLI
Copy link
Member

GreLI commented Feb 24, 2019

Merged in d23355e (there was a conflict with test from other PR).

@GreLI GreLI closed this Feb 24, 2019
@GreLI
Copy link
Member

GreLI commented Feb 24, 2019

SVGO v1.2.0 has been just released!

@amrocha amrocha mentioned this pull request Mar 4, 2019
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roblevintennis roblevintennis roblevintennis left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Herman-Freund @frebro @roblevintennis @raduflp @MattMakes @Jetski5822 @GreLI